Work towards Batcher unification
#553
Conversation
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
I think we can land this change, but there's one comment around pre-sizing chunks. At the moment, we can use a different capacity than the one from Timely, with this change we'd fall back to Timely's ensure_capacity, which may or may not be what we want here. I think this can be mitigated by using a newtype pattern.
frankmcsherry
force-pushed
the
batcher_unification
branch
from
1ad5485 to
a609da1
Compare
frankmcsherry
force-pushed
the
batcher_unification
branch
from
a609da1 to
291c129
Compare
Co-authored-by: Petros Angelatos <[email protected]>
|
Performance-wise, this PR seems to maintain parity with vs on In discussion with @antiguru we've concluded that it should be ok despite a few quirks that we think we can dial in, around buffer sizing and other nits. We'll make sure that we assess the performance on MZ, and "fail forward" from this work. |
3 participants
The
MergeBatcherimplementation is shared across multiple implementations, that re-do fairly similar work. This PR attempts to extract their differences into compact support traits they can implement, largely about how to read from and write into containers, while in the process of merging.The PR is meant to be largely behavior preserving, with some light glitches here and there around the elegance with which one can manipulate the general objects. I'm 100% up for discussing those as part of review, and either shipping or prioritizing any fixes for them. Examples include: 1. dropping some containers in the act of merging (the final containers in a chain), 2. calling
unwrap()far too often, rather than destructuring.No hurry to merge, but did want to put this in place so we can start looking at it.
Although the PR reads as fairly LOC positive, several hundred are a new
columnar.rsexample meant to exercise the non-privileged access to these traits and types. Also, the PR does not yetgit rmthe columnation merger which is now dead code, nor is it yet able to get the flat container merger to build (???) so it cannot be removed.